chore: effect rpc and layer based startup#929
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dev URL redirect drops request path and query
- Fixed the redirect to construct a new URL from the request's pathname and search params against the dev origin, and updated the test to assert the correct behavior.
Or push these changes by commenting:
@cursor push 089fb595df
Preview (089fb595df)
diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts
--- a/apps/server/src/http.ts
+++ b/apps/server/src/http.ts
@@ -90,7 +90,8 @@
const config = yield* ServerConfig;
if (config.devUrl) {
- return HttpServerResponse.redirect(config.devUrl.href, { status: 302 });
+ const devTarget = new URL(`${url.pathname}${url.search}`, config.devUrl);
+ return HttpServerResponse.redirect(devTarget.href, { status: 302 });
}
if (!config.staticDir) {
diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts
--- a/apps/server/src/server.test.ts
+++ b/apps/server/src/server.test.ts
@@ -266,7 +266,7 @@
const response = yield* Effect.promise(() => fetch(url, { redirect: "manual" }));
assert.equal(response.status, 302);
- assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/");
+ assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/foo/bar");
}).pipe(Effect.provide(NodeHttpServer.layerTest)),
);|
|
||
| const config = yield* ServerConfig; | ||
| if (config.devUrl) { | ||
| return HttpServerResponse.redirect(config.devUrl.href, { status: 302 }); |
There was a problem hiding this comment.
Dev URL redirect drops request path and query
High Severity
The dev URL redirect uses config.devUrl.href without appending the incoming request's pathname or search params. Every GET request (e.g. /settings?tab=general) redirects to the dev server root (http://127.0.0.1:5173/) instead of the matching route (http://127.0.0.1:5173/settings?tab=general). The reference implementation correctly constructs new URL(url.pathname + url.search, origin). The test at line 269 asserts the broken behavior, masking the regression.
Additional Locations (1)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) => | ||
| Option.getOrElse(Option.filter(flag, Boolean), () => envValue); |
There was a problem hiding this comment.
🟢 Low src/cli.ts:107
resolveBooleanFlag filters the flag with Option.filter(flag, Boolean), which converts Some(false) to None because Boolean(false) is false. When the user explicitly passes --no-browser=false, the explicit false is ignored and falls back to envValue. Consider removing the filter so explicit false values are preserved.
-const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) =>
- Option.getOrElse(Option.filter(flag, Boolean), () => envValue);
+const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) =>
+ Option.getOrElse(flag, () => envValue);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/cli.ts around lines 107-108:
`resolveBooleanFlag` filters the flag with `Option.filter(flag, Boolean)`, which converts `Some(false)` to `None` because `Boolean(false)` is `false`. When the user explicitly passes `--no-browser=false`, the explicit `false` is ignored and falls back to `envValue`. Consider removing the filter so explicit `false` values are preserved.
Evidence trail:
apps/server/src/cli.ts lines 107-108 (REVIEWED_COMMIT) - `resolveBooleanFlag` definition using `Option.filter(flag, Boolean)`; lines 131, 133-136, 137-140 show the function is used for `noBrowser`, `autoBootstrapProjectFromCwd`, and `logWebSocketEvents` flags.
| stream: true, | ||
| }); | ||
|
|
||
| export const WsRpcGroup = RpcGroup.make( |
There was a problem hiding this comment.
🟢 Low src/wsRpc.ts:257
WsRpcGroup omits WsSubscribeOrchestrationDomainEventsRpc and WsSubscribeServerLifecycleRpc, which are defined in the same file and follow the same pattern as included subscription RPCs. If this group is used to register available endpoints, clients will be unable to subscribe to orchestration domain events or server lifecycle events.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/contracts/src/wsRpc.ts around line 257:
`WsRpcGroup` omits `WsSubscribeOrchestrationDomainEventsRpc` and `WsSubscribeServerLifecycleRpc`, which are defined in the same file and follow the same pattern as included subscription RPCs. If this group is used to register available endpoints, clients will be unable to subscribe to orchestration domain events or server lifecycle events.
Evidence trail:
packages/contracts/src/wsRpc.ts lines 212-218 (WsSubscribeOrchestrationDomainEventsRpc definition), lines 233-238 (WsSubscribeServerLifecycleRpc definition), lines 240-287 (WsRpcGroup definition which includes WsSubscribeTerminalEventsRpc at line 279 and WsSubscribeServerConfigRpc at line 280, but omits the two mentioned RPCs)
1 participant
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Refactors core server startup and WebSocket request handling to a new Effect
RpcServer/HttpRoutercomposition, so regressions could break connectivity or endpoint behavior. Adds new path/attachment handling and command normalization that touches filesystem writes and validation logic.Overview
Migrates the server’s WebSocket request/response handling from the legacy
wsServer.tsswitch/envelope path to Effect RPC routes inapps/server/src/ws.ts, wiring many existingWS_METHODS/orchestration procedures into a sharedWsRpcGroupwith typed errors and a new integration-heavyserver.test.ts.Restructures startup into a layer-based CLI entrypoint (
src/bin.ts+src/cli.ts) and a newserver.tsthat composes HTTP routes (health, static/dev redirect, attachments) plus the WebSocket RPC route, and tightens filesystem safety viaresolveWorkspaceWritePathand orchestration command normalization for attachment persistence.Also centralizes several error types by re-exporting them from
@t3tools/contracts, updates package bin/start scripts accordingly, and adds a.plans/ws-rpc-endpoint-port-plan.mdplus formatter ignore for.reference.Written by Cursor Bugbot for commit e3ac9ed. This will update automatically on new commits. Configure here.
Note
Replace WebSocket request handling with Effect RPC layer and restructure server startup
/wsviaWsRpcLayerin ws.ts, covering orchestration, git, terminal, project file ops, keybindings, and shell open. Contracts are defined in the new wsRpc.ts.ServerLiveservice/layer pattern withServerLayer(aLayer.effectDiscard) and adds a CLI entrypoint in bin.ts with flag/env-based config resolution in cli.ts.GitManagerServiceError,TerminalError,OrchestrationDispatchCommandError,OpenError, etc.) used as typed RPC error channels.RouteRequestError; clients must migrate to the/wsRPC endpoint.📊 Macroscope summarized e3ac9ed. 38 files reviewed, 5 issues evaluated, 2 issues filtered, 1 comment posted
🗂️ Filtered Issues
.reference/server/src/model-store.ts — 0 comments posted, 1 evaluated, 1 filtered
subscribe: Events published betweencatchupstream completion andlivestream subscription start will be lost.Stream.concat(catchup, live)runscatchupfirst (line 210-212), and only after it completes doeslive(line 214-216) subscribe toeventsPubSubviaStream.fromPubSub. Any events published toeventsPubSubduring this gap (e.g., by concurrentappendcalls on line 202) will not be delivered to the subscriber. [ Out of scope (triage) ]packages/contracts/src/keybindings.test.ts — 0 comments posted, 2 evaluated, 1 filtered
Schema.decodeUnknownExitreturns anExitvalue directly, not anEffect. Usingyield*on anExitinsideEffect.genwill fail becauseExitis not a yieldableEffect. The code should either useSchema.decodeUnknownEffect(which returns anEffect) or wrap theExitwith something likeEffect.fromExit. [ Failed validation ]